Skip to content

Conversation

digital88
Copy link
Contributor

My attempt to help in #1136 (comment)

Copy link

netlify bot commented Apr 5, 2025

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 0958bf7
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-dotnet/deploys/68dbf67f5795d5000809d9a4
😎 Deploy Preview https://deploy-preview-1417--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@TheConstructor
Copy link
Contributor

Does this still work, now that #1437 was merged?

@digital88
Copy link
Contributor Author

digital88 commented May 15, 2025

Does this still work, now that #1437 was merged?

It's a script to determine which tests should be run, depending on files actually been changed in PR. #1437 still seems to run all tests?

@TheConstructor
Copy link
Contributor

Does this still work, now that #1437 was merged?

It's a script to determine which tests should be run, depending on files actually been changed in PR. #1437 still seems to run all tests?

I may have scanned to quickly across the script, I thought it would rely on the .cake-files, that the PR removed, but it only runs all tests, if they change, right?

@digital88
Copy link
Contributor Author

Does this still work, now that #1437 was merged?

It's a script to determine which tests should be run, depending on files actually been changed in PR. #1437 still seems to run all tests?

I may have scanned to quickly across the script, I thought it would rely on the .cake-files, that the PR removed, but it only runs all tests, if they change, right?

Ah, line 64 should be removed now, you are right. Yes, I assumed that if a .cake file changed, then this script output should say: "all tests should be run". This may be incorrect assumption.

The general idea of this script is use it in CI, to filter test projects which should be run. For example If PR change 1 file in Testcontainers.ActiveMq project, script would output that only single test project Testcontainers.ActiveMq.Tests should be run.

The cake task TestsTask looks like it's not filtering test projects in any way.

@digital88 digital88 closed this Aug 31, 2025
@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Sep 5, 2025

@digital88 Any particular reason you closed the PR? I think it's a valuable improvement, I just haven't had the time to review and test it yet.

@digital88
Copy link
Contributor Author

@digital88 Any particular reason you closed the PR? I think it's a valuable improvement, I just haven't had the time to review and test it yet.

Hello.

I think this PR is a bit outdated and also there exists a better tool for this task (mentioned here #1136 (comment)).

But I'm ok to reopen and refresh this PR if we choose this approach.

@digital88 digital88 reopened this Sep 7, 2025
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we'll use this script with something like #1136 (comment) to generate the JSON (strategy) at runtime, right?

Just a small thing to keep in mind: For the main and develop branches, we still need to test everything until we have dedicated projects in Sonar, otherwise the results won't be accurate.

I think the GH status checks are already set up. The "Test Report" will pass once all tests succeed, regardless of how many projects we run.

@digital88
Copy link
Contributor Author

If I understand correctly, we'll use this script with something like #1136 (comment) to generate the JSON (strategy) at runtime, right?

Just a small thing to keep in mind: For the main and develop branches, we still need to test everything until we have dedicated projects in Sonar, otherwise the results won't be accurate.

I think the GH status checks are already set up. The "Test Report" will pass once all tests succeed, regardless of how many projects we run.

Yes, the idea is to generate strategy at runtime. For example if only postgres module updated, no need to test other modules.

For this block I also made some assumptions, they may not be correct:

# ignore any files in this dirs
$excludedDirs = @(
    "examples/"
)
# if a file with extension like this was changed, we run tests for all modules
$wellKnownImportantFilesExt = @(
    ".cake",
    ".props"
)
# if a file inside this dirs was changed, we run tests for all modules
$wellKnownImportantDirs = @(
    "src/Testcontainers/"
)

Should we change something here as well?

For the main and develop branches, I think the gh action should check the brach and skip invoking script if it's not needed (we are going to test all modules anyway).

@HofmeisterAn
Copy link
Collaborator

I'll add the runtime mechanism in the next few days, including a hook to filter out projects we don't want to test with your script. After that, I'll review the script closely.

@HofmeisterAn
Copy link
Collaborator

I'm done with the prepation. I think we can just use the changed files and ignore the unrelated projects.

I believe we can use the begin block in the PowerShell script to gather all the changed files, then process each item in the process block, and filter as needed. For main and develop, we can simply return the InputObject.

Process {
# Return test projects unchanged.
$InputObject
}

@digital88
Copy link
Contributor Author

I'm done with the prepation. I think we can just use the changed files and ignore the unrelated projects.

I believe we can use the begin block in the PowerShell script to gather all the changed files, then process each item in the process block, and filter as needed. For main and develop, we can simply return the InputObject.

Process {
# Return test projects unchanged.
$InputObject
}

Anything I need to change in List-ChangedModules.ps1 script? Not quite sure if something is needed or not right now.

@HofmeisterAn
Copy link
Collaborator

Yeah, we'll need to tweak the script a bit, at least based on what I have in mind.

  • The pipeline sends each test configuration to Filter-TestProjects.ps1.
    • The input (InputObject) looks like @{name=Testcontainers; runs-on=ubuntu-24.04}.
  • The script decides whether or not that test configuration should run.
  • The Begin block runs once at the start and reads all the changed files.
  • The Process block decides for each test configuration whether it should run.

Something like:

Begin {
    # TODO: Read all changed files.
    $changedFiles = Get-ChangedFiles
}

Process {
    if (Accept -InputObject $InputObject -ChangedFiles $changedFiles) {
        $InputObject
    }
}

function Accept {
    Param (
        [string]$InputObject,
        [string[]]$ChangedFiles
    )

    return ($ChangedFiles | Where-Object { $_ -Like "*$($InputObject.name)*" }).Count -Gt 0
}

WDYT? I don't think there'll be many changes. If you're busy, I can probably take a look next week.

@digital88
Copy link
Contributor Author

digital88 commented Oct 7, 2025

Get-ChangedFiles

I think we need something like this action to list all changed files and pass them to script from "outside" via script argument or env variable.

Making changed fields as argument also improves script testability. It's hard to test if we are getting changed files inside the script itself.

@HofmeisterAn
Copy link
Collaborator

I like the approach with the env var ALL_CHANGED_FILES. My idea was setting the env var if nothings is set.

@digital88
Copy link
Contributor Author

I like the approach with the env var ALL_CHANGED_FILES. My idea was setting the env var if nothings is set.

So we need a new job step there which sets ALL_CHANGED_FILES env var? Before invoking Collect-TestProjects.ps1

@HofmeisterAn
Copy link
Collaborator

I like the approach with the env var ALL_CHANGED_FILES. My idea was setting the env var if nothings is set.

So we need a new job step there which sets ALL_CHANGED_FILES env var? Before invoking Collect-TestProjects.ps1

Yes, if we use the same mechanism as Go (I haven't looked into it before). I assumed there was something built-in in GH.

@digital88
Copy link
Contributor Author

  • The script decides whether or not that test configuration should run.

Can we define some rules when and when not configuration should run?

From the top of my head (all rules must be checked for a given input configuration):

  • If any file changed in /src/Testcontainers/** regardless of any other module changes, all test should run, so return true for any input configuration.
  • If only change is in module, for example /src/Testcontainers.ActiveMq/** run only ActiveMq tests, so return true for Testcontainers.ActiveMq and false for other inputs. Applies same logic for two or more modules.
  • If /tests directory changed, run test only for module(s) which changed.
  • Probably skip if only /docs and /examples changed

Not sure how to react on /build or /.github folder changes? Also some important files like Directory.Build.props.

@HofmeisterAn
Copy link
Collaborator

  • The script decides whether or not that test configuration should run.

Can we define some rules when and when not configuration should run?

I'm not exactly sure what you mean by that.

From the top of my head (all rules must be checked for a given input configuration):

  • If any file changed in /src/Testcontainers/** regardless of any other module changes, all test should run, so return true for any input configuration.
  • If only change is in module, for example /src/Testcontainers.ActiveMq/** run only ActiveMq tests, so return true for Testcontainers.ActiveMq and false for other inputs. Applies same logic for two or more modules.
  • If /tests directory changed, run test only for module(s) which changed.
  • Probably skip if only /docs and /examples changed

That's exactly what I had in mind. Sounds like a good way to start, and we can adjust as we go.

Not sure how to react on /build or /.github folder changes? Also some important files like Directory.Build.props.

I think the fallback should always be to run everything if no rule matches (should be exceptional cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants